Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor LibriSpeech Conformer RNN-T recipe #2366

Closed

Conversation

hwangjeff
Copy link
Contributor

@hwangjeff hwangjeff commented May 6, 2022

Modifies the example LibriSpeech Conformer RNN-T recipe as follows:

  • Moves data loading and transforms logic from lightning module to data module (improves generalizability and reusability of lightning module and data module).
  • Moves transforms logic from dataloader collator function to dataset (resolves dataloader multiprocessing issues on certain platforms).
  • Replaces lambda functions with partial equivalents (resolves pickling issues in certain runtime environments).
  • Modifies training script to allow for specifying path model checkpoint to restart training from.

trainer.fit(model)
model = ConformerRNNTModule(str(args.sp_model_path))
data_module = get_data_module(str(args.librispeech_path), str(args.global_stats_path), str(args.sp_model_path))
trainer.fit(model, data_module)
Copy link
Member

@nateanl nateanl May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be great if we add a --resume-checkpoint argument with None as default value, and change this line to

trainer.fit(model, data_module, ckpt_path=args.resume_checkpoint)

Copy link
Contributor

@xiaohui-zhang xiaohui-zhang May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--resume-from-checkpoint may sound better

@hwangjeff hwangjeff force-pushed the librispeech_conformer_rnnt_refactor branch from 0ec7ce4 to 0078c68 Compare May 6, 2022 20:34
@hwangjeff hwangjeff marked this pull request as ready for review May 11, 2022 01:49
Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stamp

from pytorch_lightning import LightningDataModule, seed_everything


seed_everything(1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right place to seed? I would imagine this happen once (and only once) at the very beginning of the CLI entry point.

seed_everything(1)


def _batch_by_token_count(idx_target_lengths, token_limit, sample_limit=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change sample_limit to batch_size and token_limit to max_tokens per our previous discussion? It's ok if you want to do that in another PR.

return dataloader

def test_dataloader(self):
dataset = torchaudio.datasets.LIBRISPEECH(self.librispeech_path, url="test-clean")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better make the eval split customizable as we discussed. again I can do that in a later PR as well.

train_transform,
val_transform,
test_transform,
max_token_limit=700,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_tokens should be fine. "max" duplicates with "limit"

val_transform,
test_transform,
max_token_limit=700,
sample_limit=2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

batch_size

@facebook-github-bot
Copy link
Contributor

@hwangjeff has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@github-actions
Copy link

Hey @hwangjeff.
You merged this PR, but labels were not properly added. Please add a primary and secondary label (See https://github.com/pytorch/audio/blob/main/.github/process_commit.py)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants